Cache Last-Modified in Redis to avoid DB hits on conditional GETs - #596

save() writes to the cache synchronously, right after super().save(), but every other cache mutation this PR adds (in comicsdb/signals.py) defers via transaction.on_commit(...).

If this save happens inside a transaction.atomic() block that later rolls back — e.g. IssueCreate.form_valid in comicsdb/views/issue.py, or AttributionCreateMixin/AttributionUpdateMixin in comicsdb/views/mixins.py, both of which save the main object and then a formset in the same atomic block — the DB change is undone but the Redis entry isn't, so it keeps serving the phantom/stale modified value for up to LAST_MODIFIED_CACHE_TTL (30 days). Combined with CachedLastModifiedMixin's cache-hit path not re-checking get_object(), this can turn into a false 304 for a row that was never actually committed.

Suggest matching the pattern used elsewhere in the PR:

def save(self, *args, **kwargs):
    super().save(*args, **kwargs)
    transaction.on_commit(lambda: set_last_modified(self))

Beyond the transaction-ordering issue above: even on a fully committed save, if this set_last_modified(self) call fails (a transient Redis blip), the cache isn't left empty — it keeps serving whatever value was cached before this save, since write-through is the only invalidation path for a direct field save (no M2M/delete signal fires).

That makes this failure mode different from a cache miss: a miss self-heals on the next conditional GET (api/views.py:154 repopulates it), but an overwrite failure produces a stale hit — nothing detects it's wrong, so a real, committed edit can silently 304 as unchanged for up to LAST_MODIFIED_CACHE_TTL (30 days), until some later save happens to succeed and overwrite it.

Given that, _safe_set probably deserves the same retry-and-escalate treatment proposed for _safe_delete_many in the thread below, rather than staying a silent best-effort write — the "populate is self-healing" assumption that makes swallowing failures safe for reads doesn't hold for this particular caller.

On a cache hit, this returns the cached timestamp directly without ever calling get_object() to confirm the row still exists in the DB.

There are two ways this can currently go stale and mask a 404 as a 304:

1. Rolled-back create — if LastModifiedCacheMixin.save() (see comment on comicsdb/models/common.py) writes to the cache before the surrounding transaction commits, a rollback leaves a phantom entry for a pk that was never actually persisted.
2. Delete invalidation failure — post_delete_last_modified in comicsdb/signals.py clears the cache via _safe_delete_many, which swallows Redis errors. If that call fails for a real, committed delete, the stale entry survives and this fast path has no way to notice.

Both cases mean a conditional GET for that pk returns 304 Not Modified instead of 404, for up to LAST_MODIFIED_CACHE_TTL (30 days) — since this path has no fallback check against the DB.

Worth considering whether the fast path should periodically re-verify via get_object(), or whether the write/invalidation side needs to guarantee it can never get out of sync with the DB in the first place.

Following up on trigger 2 above (delete invalidation failure): the fix isn't to stop swallowing the exception entirely — there's no Sentry/ADMINS email configured in this project, so letting it raise into the request would just turn a cache-consistency issue into an unhandled 500 for whoever triggered the delete, with no extra visibility to show for it.

The more useful fix is in _safe_delete_many (comicsdb/cache.py): retry a couple of times, since Redis blips are often transient, and if it still fails, escalate to ERROR instead of WARNING so it's distinguishable from the read/write-populate failures _safe_get/_safe_set tolerate. Those two can stay exactly as they are — a failed populate is self-healing on the next read, but a failed invalidation isn't; the stale entry just sits there until the TTL expires. That asymmetry is worth keeping explicit rather than reusing the same wrapper for all three.

def _safe_delete_many(keys, *, retries=3):
    """Best-effort cache invalidation, retried because a failure here — unlike
    _safe_get/_safe_set — has no self-healing path: the stale entry keeps
    serving until it's naturally overwritten or the TTL expires.
    """
    for attempt in range(1, retries + 1):
        try:
            cache.delete_many(keys)
            return
        except Exception:  # noqa: BLE001
            if attempt == retries:
                LOGGER.error(
                    "Failed to invalidate cache keys %s after %d attempts; "
                    "stale entries will serve until TTL expiry (%ds)",
                    keys, retries, LAST_MODIFIED_CACHE_TTL, exc_info=True,
                )
            else:
                LOGGER.warning(
                    "Retrying cache invalidation for %s (attempt %d/%d)",
                    keys, attempt, retries, exc_info=True,
                )

There's no compare-and-swap here, so a slower request can overwrite a fresher value a concurrent writer already cached:

• Request A (a conditional GET) reads Arc.modified = M1 from the DB at T0.
• Concurrently, request B (a PATCH) commits modified = M2, and its own save() write-through caches M2 at T1 (T0 < T1).
• Request A's write-through here fires at T2 (T1 < T2), overwriting the cache back down to the stale M1.

A client that already received Last-Modified: M2 from B's response and later polls with If-Modified-Since: M2 now finds the cache holding M1 <= M2 and gets a false 304, silently missing a real update.

set_last_modified could guard against this cheaply by only writing if the new value is newer than what's currently cached:

def set_last_modified(instance) -> None:
    modified = getattr(instance, "modified", None)
    if modified is None:
        return

    key = last_modified_cache_key(instance.__class__, instance.pk)
    current = _safe_get(key)
    if isinstance(current, int) and current >= int(modified.timestamp()):
        return

    _safe_set(key, int(modified.timestamp()), LAST_MODIFIED_CACHE_TTL)

This doesn't close the race entirely — the get-then-set pair still isn't atomic — but it shrinks the window from "however long request processing takes" down to a single round trip, which should be enough given how rarely two writes to the same row land within milliseconds of each other. A fully atomic fix would need a Redis-native compare-and-set (e.g. a Lua script or ZADD GT), which is probably more than this feature needs.

The only thing preventing this mixin from being used on a per-user-filtered viewset is this docstring — there's no runtime check enforcing it structurally.

ReadingListViewSet (below, line 668) is a concrete near-miss: its get_queryset() filters by self.request.user (public lists + own lists), which is exactly the shape this docstring warns about, and it currently avoids the mixin correctly. But nothing stops a future PR from copy-pasting the pattern from SeriesViewSet/ArcViewSet onto it — it already supports conditional GET, so it's a very plausible next candidate. If that happens, it silently reintroduces the exact cross-user 304 leak that test_conditional_request_cannot_leak_other_users_item (tests/user_collection/test_api_collection.py:85) was written to catch for CollectionViewSet specifically — but that test doesn't generalize to a new viewset.

Rather than relying on every future contributor reading this docstring, the mixin could require each subclass to explicitly declare that it's safe, and fail loudly at class-definition time (i.e. at Django startup, since urls.py imports every viewset) if that declaration is missing or wrong:

class CachedLastModifiedMixin(LastModifiedMixin):
    """Answers conditional-GET checks from Redis, skipping the DB on a cache hit.

    Every subclass must explicitly set `queryset_is_user_scoped = False` (it is
    never inherited) to opt in - a cache hit skips per-user queryset filtering,
    so combining this with a viewset whose queryset filters by request.user
    would leak other users' rows via a false 304.
    """

    def __init_subclass__(cls, **kwargs):
        super().__init_subclass__(**kwargs)

        if "queryset_is_user_scoped" not in cls.__dict__:
            raise TypeError(
                f"{cls.__name__} must explicitly declare "
                "`queryset_is_user_scoped = False` to use CachedLastModifiedMixin"
            )
        if cls.queryset_is_user_scoped:
            raise TypeError(
                f"{cls.__name__} declares queryset_is_user_scoped = True; "
                "CachedLastModifiedMixin skips per-user filtering on a cache "
                "hit and must not be combined with a user-scoped queryset"
            )

Note this is a two-part change, not a drop-in mixin edit: adding __init_subclass__ alone would break the app at startup, since none of the 9 current viewsets (ArcViewSet, CharacterViewSet, CreatorViewSet, ImprintViewSet, IssueViewSet, PublisherViewSet, SeriesViewSet, TeamViewSet, UniverseViewSet) declare the attribute yet. Each of them would also need one added line: queryset_is_user_scoped = False. That's a small one-time cost, but it converts "silently reintroduces a data leak" into "the app fails to start with a clear message" the moment someone adds the mixin to a user-scoped viewset — a much stronger guarantee than a docstring, and one that doesn't depend on a test being written or run.

There are currently two independent, uncoordinated mechanisms keeping this cache in sync with the DB: the save() override here, and hand-placed transaction.on_commit(delete_last_modified...) calls wherever comicsdb/signals.py uses .filter(...).update(modified=...) to bypass save() (lines 24-25, 31-32, 55-58, 61-62). Nothing enforces that every future .update()/bulk_update() touching a cached model's modified field remembers the paired invalidation call — miss one, and the cache goes stale silently for up to LAST_MODIFIED_CACHE_TTL (30 days).

The exact same .filter(pk=...).update(modified=...) idiom already exists in pull_list/signals.py:7, reading_lists/signals.py:7, and wish_list/signals.py:7 for models that aren't cached yet. If any of those join the cached set later (plausible — ReadingListViewSet already supports conditional GET), those handlers would silently need the same treatment, with nothing to flag it.

Rather than relying on remembering to pair every .update() with an invalidation call, a custom queryset could make .update() invalidate itself:

class LastModifiedQuerySet(models.QuerySet):
    """Auto-invalidates the Redis cache for any bulk .update() on a
    LastModifiedCacheMixin model, since .update() bypasses save()."""

    def update(self, **kwargs):
        pks = frozenset(self.values_list("pk", flat=True))
        rows_updated = super().update(**kwargs)

        if pks:
            transaction.on_commit(
                lambda model=self.model, pks=pks: delete_last_modified_many(model, pks)
            )

        return rows_updated


class LastModifiedCacheMixin(models.Model):
    """Writes `modified` to comicsdb.cache on every save(), and invalidates it
    on every .update() (which bypasses save() and would otherwise leave a
    stale entry with no self-healing path).

    Use only on models whose viewset reads it via CachedLastModifiedMixin.
    """

    objects = LastModifiedQuerySet.as_manager()

    class Meta:
        abstract = True

    def save(self, *args, **kwargs):
        super().save(*args, **kwargs)
        set_last_modified(self)

This closes the gap for any future .update() call automatically, and as a bonus it lets the four existing hand-paired transaction.on_commit(delete_last_modified...) calls in comicsdb/signals.py be deleted — .update() now invalidates itself.

One prerequisite: Issue (comicsdb/models/issue.py:86-88) would need its objects, graphic_novels, and tpb custom managers removed — a repo-wide grep (including tests) turns up zero references to any of them outside their own definitions, so they look like dead code. Removing them would let Issue drop its objects override entirely and inherit LastModifiedQuerySet.as_manager() from this mixin cleanly, same as every other cached model.

This calls the viewset's overridden get_queryset() on every conditional request just to read .model — but .model is static and already known via the class-level queryset attribute every one of the 9 cached viewsets declares (e.g. queryset = Arc.objects.all()).

For IssueViewSet, SeriesViewSet, CharacterViewSet, TeamViewSet, UniverseViewSet, and ImprintViewSet, get_queryset() is overridden with several select_related/prefetch_related chains (IssueViewSet's alone has 5 select_related fields, 7 prefetch_related entries including two nested Prefetch objects, plus Avg/Count annotations). None of that triggers a DB round trip since querysets are lazy, but constructing all those queryset/expression objects on every single conditional GET — including cache hits, which this feature is specifically trying to make cheap — is wasted CPU work.

self.queryset.model gets the same class with none of that construction:

cached = get_last_modified(self.queryset.model, pk) if pk else None

pre_clear already computes pk_set and has everything needed to do the update and schedule the cache invalidation — but it defers to post_clear instead, smuggling pk_set across the two separate signal dispatches via a private _cleared_pks attribute stashed on the live model instance (write it here, read-and-delete it in the post_clear branch below).

That handoff buys nothing: .update() on parent_model doesn't touch the M2M through table this clear() call is deleting, so there's no ordering reason it can't happen immediately in pre_clear, where pk_set is already a local variable. Django dispatches pre_clear/post_clear synchronously inside the same transaction.atomic() block that wraps the through-row deletion, so doing the work in pre_clear instead doesn't change any transactional behavior — it just removes the need to mutate instance state and read it back in a second, separate call.

def update_related_modified(parent_model, field_name, instance, action, pk_set):
    """Shared logic for M2M pre_clear/post_add/post_remove/post_clear on Arc, Character, Team."""
    if action not in ("pre_clear", "post_add", "post_remove", "post_clear"):
        return

    from comicsdb.models import Issue  # noqa: PLC0415

    if isinstance(instance, Issue):
        if action == "post_clear":
            return  # already handled below, during pre_clear

        if action == "pre_clear":
            pk_set = set(getattr(instance, field_name).values_list("pk", flat=True))

        if pk_set:
            parent_model.objects.filter(pk__in=pk_set).update(modified=timezone.now())
            transaction.on_commit(
                lambda pks=frozenset(pk_set): delete_last_modified_many(parent_model, pks)
            )
    elif action != "pre_clear":
        # instance is the parent (e.g. arc.issues.add/clear(...))
        parent_model.objects.filter(pk=instance.pk).update(modified=timezone.now())
        transaction.on_commit(lambda pk=instance.pk: delete_last_modified(parent_model, pk))

This drops _cleared_pks entirely, along with the getattr/hasattr/del dance in the current post_clear branch. It'd also mean updating tests/comicsdb/test_signals.py:128-143 (test_update_related_modified_pre_clear_then_post_clear_from_issue), which currently asserts on _cleared_pks directly — that test would instead assert the update happens after pre_clear alone, with post_clear remaining a no-op.

get_last_modified reads the cached value as an int and converts it to a datetime here; the one production caller (api/views.py:148) then immediately converts it back to an int via int(cached.timestamp()) just to compare against if_modified_since — which is itself already an int, from parse_http_date_safe. That's two conversions per cache hit to compare two numbers that started and ended as ints.

(Django's condition decorator does require the final return value of _last_modified to be a real datetime — it calls .timestamp()/timezone.is_aware() on it directly — so that conversion can't be dropped entirely, but it only needs to happen once, on an actual hit, not before every comparison.)

Splitting the epoch read from the datetime conversion would remove the redundant round trip and let the comparison happen entirely in int space, matching parse_http_date_safe's own representation:

def get_last_modified_epoch(model, pk) -> int | None:
    """Cached `modified` for `model`/`pk` as epoch seconds, or None on a miss."""
    value = _safe_get(last_modified_cache_key(model, pk))
    return value if isinstance(value, int) else None


def get_last_modified(model, pk) -> datetime | None:
    """Cached `modified` for `model`/`pk`, or None on a miss."""
    epoch = get_last_modified_epoch(model, pk)
    return datetime.fromtimestamp(epoch, tz=UTC) if epoch is not None else None

get_last_modified stays as-is for its existing callers (it's used directly in tests/comicsdb/test_cache.py and tests/comicsdb/test_api_conditional_requests.py, comparing against datetime values). Only api/views.py's cache-hit check would switch to the epoch variant:

if if_modified_since is not None:
    pk = self.kwargs.get(self.lookup_url_kwarg or self.lookup_field)
    cached_epoch = get_last_modified_epoch(self.queryset.model, pk) if pk else None

    if cached_epoch is not None and cached_epoch <= if_modified_since:
        return datetime.fromtimestamp(cached_epoch, tz=UTC)

(assumes the self.queryset.model fix suggested above, and adds a from datetime import UTC, datetime import to api/views.py.)

Every other signal connection in ready() is explicit and per-model (pre_delete.connect(pre_delete_image, sender=arc, ...), repeated by hand for character, creator, issue, publisher, team, variant), so scanning this file alone tells you which models participate in a given signal. This block breaks that convention — answering "which models get post_delete_last_modified?" now means grepping comicsdb/models/*.py for LastModifiedCacheMixin instead of reading apps.py, where every other answer to "which models get signal X?" lives.

That's a reasonable trade-off, not a bug — the loop is what lets a 10th cached model get wired automatically without an apps.py edit, which the explicit style can't do, and that's worth keeping. It's just a different registration strategy sitting unexplained next to seven examples of the other one, which could read as an oversight rather than a deliberate choice.

Worth extending the existing comment on the line above to say so explicitly, e.g.:

# Clear the cache entry on delete, so a removed row 404s instead of 304ing.
# Auto-registered (unlike the explicit connects above) so new LastModifiedCacheMixin
# models don't need an apps.py edit to get invalidation wired up.
from comicsdb.models.common import LastModifiedCacheMixin  # noqa: PLC0415

for model in self.get_models():
    if issubclass(model, LastModifiedCacheMixin):
        post_delete.connect(
            post_delete_last_modified,
            sender=model,
            dispatch_uid=f"post_delete_last_modified_{model._meta.model_name}",
        )